Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Mingw64 build and add a CI job to test it #3132

Merged
merged 6 commits into from
Jul 30, 2021

Conversation

jeromerobert
Copy link
Contributor

@jeromerobert jeromerobert commented Jul 18, 2021

This is a tentative to fix mingw64 build and add a CI job to test it. So far it does not work. I would need some help or explanation to fix it.

  • A __declspec(dllexport) symbol cannot be hidden, it must have the default visibility (this is required by gcc). 3ac690b88 make builtin_exception __declspec(dllexport) while the pybind11 namespace is visibility(hidden).
  • I get a huge amount of deprecation like warning: 'pybind11::object::object(pybind11::handle, bool)' is deprecated: Use reinterpret_borrow<object>() or reinterpret_steal<object>()

This is the follow-up.

Suggested changelog entry:

* Fix Mingw64 and add to the CI testing matrix.

@jeromerobert
Copy link
Contributor Author

A __declspec(dllexport) symbol cannot be hidden, it must have the default visibility (this is required by gcc). 3ac690b88 make builtin_exception __declspec(dllexport) while the pybind11 namespace is visibility(hidden).

An example from the GHA log:

D:/a/pybind11/pybind11/include/pybind11/detail/common.h:735:23: error: 'dllexport' implies default visibility, but 'class pybind11::builtin_exception' has already been declared with a different visibility
  735 | class PYBIND11_EXPORT builtin_exception : public std::runtime_error {
      |                       ^~~~~~~~~~~~~~~~~

Should I disable exception dllexport on Mingw64 or make the pybind11 namespace default visible ? Would @oraluben know ?

@oraluben
Copy link
Contributor

Sorry I have few idea about the issue here, I only tested against gcc on Linux (with __attribute__), I guess the root cause could be GCC handling __declspec(dllexport) and __attribute__(visibility(hidden)) differently?

If that's the case then I think disable exception dllexport on Mingw64 (or any other gcc build on Windows) is a workaround good enough.

@jeromerobert jeromerobert marked this pull request as ready for review July 29, 2021 09:02
@jeromerobert
Copy link
Contributor Author

May be CMAKE_INTERPROCEDURAL_OPTIMIZATION=OFF should moved to pybind11NewTools.cmake (i.e. LTO disabled by default for MINGW). What's your feeling ?

@henryiii
Copy link
Collaborator

I think we should handle it in pybind11Common, when we add the LTO flags. I"m trying just scaling it back a little first.

@henryiii
Copy link
Collaborator

Eventually, we might be able to come up with the correct LTO flags for MinGW, but for now, I think this is a clear improvement.

@henryiii henryiii requested review from Skylion007 and rwgk July 29, 2021 15:03
include/pybind11/detail/common.h Outdated Show resolved Hide resolved
include/pybind11/pytypes.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the logs ... a ton of errors, wow.

include/pybind11/detail/common.h Outdated Show resolved Hide resolved
include/pybind11/detail/common.h Show resolved Hide resolved
jeromerobert and others added 5 commits July 30, 2021 08:46
This is a fix for errors like:

D:/a/pybind11/pybind11/include/pybind11/detail/common.h:735:23: error: 'dllexport' implies default visibility, but 'class pybind11::builtin_exception' has already been declared with a different visibility
  735 | class PYBIND11_EXPORT builtin_exception : public std::runtime_error {
      |                       ^~~~~~~~~~~~~~~~~
@jeromerobert
Copy link
Contributor Author

jeromerobert commented Jul 30, 2021

Oops it looks like a pushed PYBIND11_EXPORT_EXCEPTION without having read the last hours discussions.

@jeromerobert
Copy link
Contributor Author

Fortunately this seems to be what you expect.

I looked at the logs ... a ton of errors, wow.

You mean tons of warnings ? Yes, I don't understand why (and where) object(handle h, bool is_borrowed) is called.

@rwgk
Copy link
Collaborator

rwgk commented Jul 30, 2021

Fortunately this seems to be what you expect.

I looked at the logs ... a ton of errors, wow.

You mean tons of warnings ?

On all (or at least most) other platforms we're using -Werror or equivalent.

Yes, I don't understand why (and where) object(handle h, bool is_borrowed) is called.

Do you have an interactive setup? A trick I often use is to add some extra arg(s) to the function, then build with --keep-going, it'll show all callers. Doesn't always work, but this looks like it could.

-object(handle h, bool is_borrowed)
+object(handle h, bool is_borrowed, const std::string &)

@jeromerobert
Copy link
Contributor Author

I tried to apply the following patch which remove all deprecated member. All deprecation warnings disappeared and I get no error.

diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h
index 20d119f..b65ecd8 100644
--- a/include/pybind11/attr.h
+++ b/include/pybind11/attr.h
@@ -41,8 +41,6 @@ struct sibling { handle value; sibling(const handle &value) : value(value.ptr())
 /// Annotation indicating that a class derives from another given type
 template <typename T> struct base {

-    PYBIND11_DEPRECATED("base<T>() was deprecated in favor of specifying 'T' as a template argument to class_")
-    base() { } // NOLINT(modernize-use-equals-default): breaks MSVC 2015 when adding an attribute
 };

 /// Keep patient alive while nurse lives
@@ -61,10 +59,6 @@ struct buffer_protocol { };
 struct metaclass {
     handle value;

-    PYBIND11_DEPRECATED("py::metaclass() is no longer required. It's turned on by default now.")
-    // NOLINTNEXTLINE(modernize-use-equals-default): breaks MSVC 2015 when adding an attribute
-    metaclass() {}
-
     /// Override pybind11's default metaclass
     explicit metaclass(handle value) : value(value) { }
 };
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 139a411..a04f9aa 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -984,16 +984,6 @@ class module_ : public object {
 public:
     PYBIND11_OBJECT_DEFAULT(module_, object, PyModule_Check)

-    /// Create a new top-level Python module with the given name and docstring
-    PYBIND11_DEPRECATED("Use PYBIND11_MODULE or module_::create_extension_module instead")
-    explicit module_(const char *name, const char *doc = nullptr) {
-#if PY_MAJOR_VERSION >= 3
-        *this = create_extension_module(name, doc, new PyModuleDef());
-#else
-        *this = create_extension_module(name, doc, nullptr);
-#endif
-    }
-
     /** \rst
         Create Python binding for a new function within the module scope. ``Func``
         can be a plain C++ function, a function pointer, or a lambda function. For
diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h
index 4cf606e..3fa36e4 100644
--- a/include/pybind11/pytypes.h
+++ b/include/pybind11/pytypes.h
@@ -232,8 +232,6 @@ protected:
 class object : public handle {
 public:
     object() = default;
-    PYBIND11_DEPRECATED("Use reinterpret_borrow<object>() or reinterpret_steal<object>()")
-    object(handle h, bool is_borrowed) : handle(h) { if (is_borrowed) inc_ref(); }
     /// Copy constructor; always increases the reference count
     object(const object &o) : handle(o) { inc_ref(); }
     /// Move constructor; steals the object from ``other`` and preserves its reference count
@@ -854,8 +852,6 @@ PYBIND11_NAMESPACE_END(detail)
 //       isn't even used.
 #define PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \
     public: \
-        PYBIND11_DEPRECATED("Use reinterpret_borrow<"#Name">() or reinterpret_steal<"#Name">()") \
-        Name(handle h, bool is_borrowed) : Parent(is_borrowed ? Parent(h, borrowed_t{}) : Parent(h, stolen_t{})) { } \
         Name(handle h, borrowed_t) : Parent(h, borrowed_t{}) { } \
         Name(handle h, stolen_t) : Parent(h, stolen_t{}) { } \
         PYBIND11_DEPRECATED("Use py::isinstance<py::python_type>(obj) instead") \
@@ -1292,8 +1288,6 @@ public:
 class capsule : public object {
 public:
     PYBIND11_OBJECT_DEFAULT(capsule, object, PyCapsule_CheckExact)
-    PYBIND11_DEPRECATED("Use reinterpret_borrow<capsule>() or reinterpret_steal<capsule>()")
-    capsule(PyObject *ptr, bool is_borrowed) : object(is_borrowed ? object(ptr, borrowed_t{}) : object(ptr, stolen_t{})) { }

     explicit capsule(const void *value, const char *name = nullptr, void (*destructor)(PyObject *) = nullptr)
         : object(PyCapsule_New(const_cast<void *>(value), name, destructor), stolen_t{}) {
@@ -1301,13 +1295,6 @@ public:
             pybind11_fail("Could not allocate capsule object!");
     }

-    PYBIND11_DEPRECATED("Please pass a destructor that takes a void pointer as input")
-    capsule(const void *value, void (*destruct)(PyObject *))
-        : object(PyCapsule_New(const_cast<void*>(value), nullptr, destruct), stolen_t{}) {
-        if (!m_ptr)
-            pybind11_fail("Could not allocate capsule object!");
-    }
-
     capsule(const void *value, void (*destructor)(void *)) {
         m_ptr = PyCapsule_New(const_cast<void *>(value), nullptr, [](PyObject *o) {
             auto destructor = reinterpret_cast<void (*)(void *)>(PyCapsule_GetContext(o));

@jeromerobert
Copy link
Contributor Author

Here is the very first warning:

C:/msys2/home/jerome/pybind11/include/pybind11/pytypes.h: In constructor 'pybind11::object::object(pybind11::handle, bool)':
C:/msys2/home/jerome//pybind11/include/pybind11/pytypes.h:236:82: warning: 'pybind11::object::object(pybind11::handle, bool)' is deprecated: Use reinterpret_borrow<object>() or reinterpret_steal<object>() [-Wdeprecated
-declarations]
  236 |     object(handle h, bool is_borrowed) : handle(h) { if (is_borrowed) inc_ref(); }
      |                                                                                  ^
C:/msys2/home/jerome/pybind11/include/pybind11/pytypes.h:236:5: note: declared here
  236 |     object(handle h, bool is_borrowed) : handle(h) { if (is_borrowed) inc_ref(); }
      |     ^~~~~~

It looks that a warning is triggered each time a deprecated member is declared whether it is used or not. That does not make sens. Also, I cannot reproduce that on simple example.

@rwgk
Copy link
Collaborator

rwgk commented Jul 30, 2021

I'd be fine with #if !defined(__MINGW32__) around the deprecated code.

@henryiii
Copy link
Collaborator

henryiii commented Jul 30, 2021

It looks that a warning is triggered each time a deprecated member is declared whether it is used or not. That does not make sense.

I noticed this too, and don't know why it's behaving this way.

I'd be fine with #if !defined(MINGW32) around the deprecated code.

As long as you get the tests to pass, I'd be okay with that too. Though this seems like a weird bug, I wonder if there's a flag we are passing that's affecting this. @jeromerobert have you compared the compile commands we use with your simple example?

(Just leaving it for now is also okay, or doing it in a follow up)

It trigger many warnings for unknown reasons
@jeromerobert
Copy link
Contributor Author

I'd be fine with #if !defined(MINGW32) around the deprecated code.

I disabled PYBIND11_DEPRECATED on mingw32

have you compared the compile commands we use with your simple example

Yes. I copied them from the verbose make log.

@henryiii
Copy link
Collaborator

henryiii commented Jul 30, 2021

I disabled PYBIND11_DEPRECATED on mingw32

Actually that's the simplest and (in hindsight) obvious solution. :)

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poke me if I don't notice when this finishes, and I'll merge & backport to the v2.7 branch.

@jeromerobert
Copy link
Contributor Author

All green !

@henryiii henryiii merged commit 9e8a741 into pybind:master Jul 30, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 30, 2021
henryiii added a commit that referenced this pull request Jul 30, 2021
* mingw64 platform string is like mingw_xxx not "mingw"

See https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-python/0099-Change-the-get_platform-method-in-sysconfig-and-dist.patch

* Mingw: Do not dllexport exceptions

This is a fix for errors like:

D:/a/pybind11/pybind11/include/pybind11/detail/common.h:735:23: error: 'dllexport' implies default visibility, but 'class pybind11::builtin_exception' has already been declared with a different visibility
  735 | class PYBIND11_EXPORT builtin_exception : public std::runtime_error {
      |                       ^~~~~~~~~~~~~~~~~

* GHA: Test Mingw64 build

* fix: avoid thin binaries on mingw

* fix: drop lto on MinGW

* Mingw64: disable PYBIND11_DEPRECATED

It trigger many warnings for unknown reasons

Co-authored-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants